Add test for feat_req__persistency__multiple_kvs #73
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new integration test for verifying that multiple KVS (Key-Value Store) instances with different IDs can store and retrieve independent values without interference. The implementation includes both Rust test scenarios and Python test cases, along with necessary helper modules for KVS instance management and parameter handling.
Key changes:
- New test scenario
multiple_instance_idsto verify multiple KVS instances operate independently - Helper modules for KVS parameter parsing and instance creation
- Refactored code organization by moving runtime helpers into a
kyronsubdirectory - Extracted common test property decorator into a shared module
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
feature_integration_tests/rust_test_scenarios/src/scenarios/persistency/multiple_kvs_per_app.rs |
Implements the Rust test scenario that creates two KVS instances, stores different values, and verifies they persist independently |
feature_integration_tests/rust_test_scenarios/src/scenarios/persistency/mod.rs |
Defines the persistency scenario group containing the multiple instance IDs test |
feature_integration_tests/rust_test_scenarios/src/scenarios/mod.rs |
Registers the new persistency scenario group with the root scenario group |
feature_integration_tests/rust_test_scenarios/src/scenarios/basic/orchestration_with_persistency.rs |
Updates import path to reflect reorganized runtime helper location |
feature_integration_tests/rust_test_scenarios/src/scenarios/basic/mod.rs |
Simplifies code by importing OrchestrationWithPersistency directly |
feature_integration_tests/rust_test_scenarios/src/internals/persistency/mod.rs |
Exports KVS helper modules for test scenarios |
feature_integration_tests/rust_test_scenarios/src/internals/persistency/kvs_parameters.rs |
Provides serde-compatible structure for deserializing KVS configuration parameters |
feature_integration_tests/rust_test_scenarios/src/internals/persistency/kvs_instance.rs |
Helper function to create KVS instances from test parameters |
feature_integration_tests/rust_test_scenarios/src/internals/mod.rs |
Reorganizes internals by adding kyron and persistency modules |
feature_integration_tests/rust_test_scenarios/src/internals/kyron/mod.rs |
New module containing Kyron-related helpers including runtime helper |
feature_integration_tests/python_test_cases/tests/persistency/multiple_kvs_per_app.py |
Python test case that verifies the multiple KVS instances scenario |
feature_integration_tests/python_test_cases/tests/basic/test_orchestartion_with_persistency.py |
Refactors to use shared test properties decorator |
feature_integration_tests/python_test_cases/test_properties.py |
Extracts test properties decorator into shared module for reuse |
feature_integration_tests/python_test_cases/fit_scenario.py |
Adds ResultCode constants for better readability of exit codes |
feature_integration_tests/python_test_cases/BUILD |
Updates build configuration to include new test_properties module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub struct MultipleInstanceIds; | ||
|
|
||
| impl Scenario for MultipleInstanceIds { | ||
| fn name(&self) -> &str { | ||
| "multiple_instance_ids" |
There was a problem hiding this comment.
The scenario name multiple_instance_ids doesn't clearly convey that this test verifies multiple KVS instances per application. Consider renaming to multiple_kvs_per_app to match the module name and better describe what's being tested.
| pub struct MultipleInstanceIds; | |
| impl Scenario for MultipleInstanceIds { | |
| fn name(&self) -> &str { | |
| "multiple_instance_ids" | |
| pub struct MultipleKvsPerApp; | |
| impl Scenario for MultipleKvsPerApp { | |
| fn name(&self) -> &str { | |
| "multiple_kvs_per_app" |
| pub struct MultipleInstanceIds; | ||
|
|
||
| impl Scenario for MultipleInstanceIds { |
There was a problem hiding this comment.
The struct name MultipleInstanceIds is inconsistent with its scenario name multiple_instance_ids and doesn't clearly indicate it's testing multiple KVS instances per application. Consider renaming to MultipleKvsPerApp to match the module name and improve clarity.
| pub struct MultipleInstanceIds; | |
| impl Scenario for MultipleInstanceIds { | |
| pub struct MultipleKvsPerApp; | |
| impl Scenario for MultipleKvsPerApp { |
| /// Parse `KvsParameters` from `Value`. | ||
| /// `Value` is expected to contain `kvs_parameters` field. | ||
| pub fn from_value(value: &serde_json::Value) -> Result<Self, serde_json::Error> { |
There was a problem hiding this comment.
The from_value method expects a nested kvs_parameters field within the provided value, which creates an inconsistent API. The method already receives a value parameter, so requiring another kvs_parameters key inside it is confusing. Consider either removing the nested access or renaming the method to clarify this expectation.
| /// Parse `KvsParameters` from `Value`. | |
| /// `Value` is expected to contain `kvs_parameters` field. | |
| pub fn from_value(value: &serde_json::Value) -> Result<Self, serde_json::Error> { | |
| /// Parse `KvsParameters` directly from a JSON `Value`. | |
| /// | |
| /// The provided `value` is expected to contain the fields of `KvsParameters` | |
| /// at its top level (i.e., it should serialize exactly as `KvsParameters`). | |
| pub fn from_value(value: &serde_json::Value) -> Result<Self, serde_json::Error> { | |
| serde_json::from_value(value.clone()) | |
| } | |
| /// Parse `KvsParameters` from a JSON `Value` that wraps it in a | |
| /// `kvs_parameters` field. | |
| /// | |
| /// This is useful when the configuration JSON has a structure like: | |
| /// | |
| /// `{ "kvs_parameters": { ...fields of KvsParameters... } }` | |
| pub fn from_wrapped_value(value: &serde_json::Value) -> Result<Self, serde_json::Error> { |
|
The created documentation from the pull request is available at: docu-html |
1 participant
No description provided.